Added ConfSoilMoisture.py and some edits in Variable.py and ilamblib.py by ypwong22 · Pull Request #44 · rubisco-sfa/ILAMB

ypwong22 · 2021-09-20T20:53:42Z

No description provided.

nocollier

I have taken a look at this. It is a tremendous amount of work and very cleanly implemented. I am a little uncomfortable that so much of it changes things that can affect how ILAMB currently runs. As maybe you are realizing now, as you add models, you find lots of issues with the differences in how models represent things. Much of my code was the way it was to handle weird differences across models. So seeing things that change parts that assume a noleap calendar, makes me nervous because it took a huge amount of work to make the codebase work for all the models.

In the future, if you plan to make changes to parts of the code that are deep (like Variable), let's talk about it so we can make sure that our API stays consistent and that we don't break other areas.

I am inclined to proceed in the following way:

take a look at my comments in this review and please answer or change as appropriate
let's remove the YW's and non-meaningful comments (like where code is commented out)
after this step I will run through my tests and make sure we aren't failing
then I will run a full analysis (like the CMIP5v6) we have, and make sure everything works as it did before

Presuming all this passes ok, then we will merge this and move on. I have been developing a new version of ILAMB for some time based on xarray. So eventually I will grab this and port it.

nocollier · 2022-08-04T15:32:33Z

bin/ilamb-run

    return C

+def MatchSensitivityConfrontation(C):
+    # YW


Let's remove the #YW comments here. Your name will be associated with this PR and thus we will always know who to blame :D

nocollier · 2022-08-04T15:35:26Z

bin/ilamb-run

+                    found = True
+    return C
+
+"""


Let's remove these lines, thanks for noticing that the function isn't needed. It seems like I got rid of it at some point.

nocollier · 2022-08-04T16:01:14Z

src/ILAMB/Scoreboard.py

        self.plot_unit           = None
        self.space_mean          = True
        self.relationships       = None
+        self.sensitivities       = None # YW


let's remove the YW

nocollier · 2022-08-04T16:17:11Z

src/ILAMB/Confrontation.py

        # Make sure the source data exists
-
-        if not os.path.isfile(self.source):
+        try:


Is there a reason that what I had wasn't working? I think the isfile is more portable across other systems?

Your current version should be fine. I did not modify this line. The os.stat() line is a carryover from ILAMB 2.5, which I based my changes on.

nocollier · 2022-08-04T16:23:55Z

src/ILAMB/Confrontation.py

-                y0 = tdata[0].year
-                yf = tdata[1].year
-
+                    t  = dataset.variables[t.bounds][...]


Was what I had in here not working for some data? The issue is that your math here assumes that the calendar is noleap and the datum is 1850-01-01 and that isn't always the case at all. In the future version of ILAMB this will matter less as we will use xarray as the guts.

This is also a carryover from ILAMB 2.5.

nocollier · 2022-08-04T16:28:43Z

src/ILAMB/Confrontation.py

-                               cmap = 'plasma' if 'plasma' in plt.cm.cmap_d else 'summer')
+                               norm = LogNorm(),
+                               cmap = 'plasma' if 'plasma' in plt.cm.cmap_d else 'summer',
+                               vmin = 1e-4, vmax = 1e-1)


Was there some problem with the way I had this? Sometimes things like this are this way to avoid warnings in the output.

This is also just a carryover from ILAMB 2.5.

nocollier · 2022-08-04T16:42:40Z

src/ILAMB/Variable.py

            assert variable_name is not None
            t0 = keywords.get("t0",None)
            tf = keywords.get("tf",None)
+            z0 = keywords.get("z0",None)


I had the time range in the Variable constructor because time is the leading dimension in netcdf files and only reading in what you need is a huge savings in memory I/O. If these depth bounds are in the constructor, why not also lat and lon?

I see. Should I remove the lines 269-270 z0 & zf statements and the z0 & zf arguments into il.FromNetCDF4, and test if things work?

Let's just leave it.

nocollier · 2022-08-04T16:43:01Z

src/ILAMB/Variable.py

            # Make sure that the value lies within the bounds
            assert np.all((self.lat>=self.lat_bnds[:,0])*(self.lat<=self.lat_bnds[:,1]))
            assert np.all((self.lon>=self.lon_bnds[:,0])*(self.lon<=self.lon_bnds[:,1]))
+            ##DEBUG


let's delete these comments

ypwong22 added 17 commits March 29, 2021 00:45

add soil moisture data; buggy

d423f5a

Relaxed NBPplot constraint to accept one model & >10 year difference

2552995

blank plots bug

da6c8b0

working now for one model

79c36dc

speed up WIP

2e13bcc

confrontation buggy

a57c713

default relationships working w/ several models

cf2d457

un-edited ConfNBP

f59de76

begin sensitivity addition

0d2314e

trend & sensitivity WIP

841fcda

added trend & sens, first trial

925dade

sens & trend framework finished

97e0222

trend sensitivity WIP

85ff1e9

sensitivity working; need to tweak visuals

adfe0ef

partialcorr done

fa11b65

fixed dropdown scores

a11db71

fixed rel scoring

02d66d9

ypwong22 marked this pull request as draft April 22, 2022 01:52

ypwong22 closed this Apr 22, 2022

ypwong22 reopened this Apr 22, 2022

multimodel

80bf6a9

nocollier reviewed Aug 4, 2022

View reviewed changes

nocollier mentioned this pull request Aug 10, 2022

Discussion Points for 2022/08/10 Group Meeting rubisco-sfa/ILAMB-Data#36

Closed

5 tasks

Conversation

ypwong22 commented Sep 20, 2021

Uh oh!

nocollier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ypwong22 Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ypwong22 Aug 15, 2022 •

edited

Loading